Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adds missing reswap modifier and adds reswap builder along with StopPolling #23

Merged
merged 8 commits into from
May 1, 2024

Conversation

tanczosm
Copy link
Collaborator

This commit introduces several enhancements to the HtmxResponse class. It adds a new optional parameter modifier to the Reswap method, allowing for more flexible response swapping. A new overload of the Reswap method is also added that takes a SwapStyleBuilder object as an argument to allow for fluent modifiers.

See Reswap fluent modifiers docs: https://jalexsocial.github.io/rizzy.docs/docs/htmx/response/#applying-fluent-modifiers

A new method, StopPolling, has been introduced which sets the response code to stop polling.

Two new classes, HtmxStatusCodes and SwapStyleBuilder, have been created. The former provides status codes specific to HTMX responses while the latter aids in constructing reswap commands.

Additionally, a new enum called ScrollDirection has been added for specifying scroll direction values for reswaps.

Copy link
Owner

@egil egil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure about this. It's not necessarily a bad idea to to have a builder that helps users create the right combination of swap modifiers. However, I want to hold off on this for a little until I better understand htmx myself.

/// <returns>This <see cref="HtmxResponse"/> object instance.</returns>
public HtmxResponse Reswap(SwapStyle swapStyle)
public HtmxResponse Reswap(SwapStyle swapStyle, string? modifier = null)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be compatible with hx-swap="show:none"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Took a dive into the htmx source. It seems like modifiers can be used without necessarily specifying a swap style. The swap specification is loaded over top of the defaults. It defaults swapStyle to innerHTML if boosted, otherwise whatever the configured defaultSwapStyle is. Then reswap can come in and change the swapstyle and/or add modifiers. Modifiers can be used independently of Swapstyle.

I'm going to overload Reswap with just a string parameter to allow modifiers only.

src/Htmxor/Http/HtmxResponse.cs Show resolved Hide resolved
@tanczosm
Copy link
Collaborator Author

I added the Reswap overload.

Some comments on the builder. It's patterned after a builder that is widely used with the go programming language:
https://pkg.go.dev/github.com/angelofallars/htmx-go#readme-swap-strategy

The difference is that they have an additional swapstyle option called "Default" that is used in cases where there is no swap style used in the reswap. Since SwapStyle is used in configurations and AjaxRequest you'd need to write a custom serializer to include "Default" as a SwapStyle option, even if semantically probably a correct option.

I updated the SwapStyleBuilder and added a suite of tests to validate the builder. I also rewrote it to override existing modifiers if accidentilly calling the same modifier. It's worth reconsidering because on the server side for reswaps you have to construct all the modifiers in the correct format by hand which is more error prone than using the builder.

You can always consider it experimental for the time being and toss if you end up not liking it as it doesn't interfere with using the other Reswap methods as it has a pretty self-contained footprint.

@tanczosm tanczosm force-pushed the mat-response-additions branch from 75f4ef6 to 9cba39e Compare April 30, 2024 17:39
Copy link
Owner

@egil egil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think you have a good point about including this in the package. Lets iterate on the API a bit though!

src/Htmxor/Http/SwapStyleBuilder.cs Show resolved Hide resolved
src/Htmxor/Http/SwapStyleBuilder.cs Outdated Show resolved Hide resolved
src/Htmxor/Http/SwapStyleBuilder.cs Outdated Show resolved Hide resolved
src/Htmxor/Http/SwapStyleBuilder.cs Show resolved Hide resolved
test/Htmxor.Tests/Http/SwapStyleBuilderTests.cs Outdated Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job adding tests. Much appreciated.

For this project I want to standardize on FluentAssertions, so please use it for assertions.

@egil
Copy link
Owner

egil commented Apr 30, 2024

The difference is that they have an additional swapstyle option called "Default" that is used in cases where there is no swap style used in the reswap. Since SwapStyle is used in configurations and AjaxRequest you'd need to write a custom serializer to include "Default" as a SwapStyle option, even if semantically probably a correct option.

Can you elaborate more on what this means (remember I am an htmx newbie).

Could we add a Default option to the SwapStyle enum to achieve this?

@egil
Copy link
Owner

egil commented Apr 30, 2024

Ps. do also rebase your branch on main. Did a major clean up this evening.

… StopPolling

This commit introduces several enhancements to the `HtmxResponse` class. It adds a new optional parameter `modifier` to the `Reswap` method, allowing for more flexible response swapping. A new overload of the `Reswap` method is also added that takes a `SwapStyleBuilder` object as an argument.

A new method, `StopPolling`, has been introduced which sets the response code to stop polling.

Two new classes, `HtmxStatusCodes` and `SwapStyleBuilder`, have been created. The former provides status codes specific to HTMX responses while the latter aids in constructing swap style command strings for HTMX responses.

Additionally, a new enum called `ScrollDirection` has been added for specifying scroll direction values.

refactor: Add Reswap method and refactor SwapStyleBuilder

Added a new Reswap method to the HtmxResponse class, allowing for more flexible response swapping. Refactored the SwapStyleBuilder class to use an OrderedDictionary for storing modifiers instead of a List, improving performance and readability. Also added nullability support for the SwapStyle property in SwapStyleBuilder. Added comprehensive unit tests for these changes in SwapStyleBuilderTests.cs.

feat: Add ShowNone method to SwapStyleBuilder

A new method, ShowNone, has been added to the SwapStyleBuilder class. This method turns off scrolling after a swap and allows for chaining with other methods in the class. Corresponding unit test has also been added to ensure that it returns the correct value.
@tanczosm
Copy link
Collaborator Author

The difference is that they have an additional swapstyle option called "Default" that is used in cases where there is no swap style used in the reswap. Since SwapStyle is used in configurations and AjaxRequest you'd need to write a custom serializer to include "Default" as a SwapStyle option, even if semantically probably a correct option.

Can you elaborate more on what this means (remember I am an htmx newbie).

Could we add a Default option to the SwapStyle enum to achieve this?

Having a "Default" option in SwapStyle could have meaning in several contexts but in many of them it is simply a no-op of sorts:

  • For configurations setting DefaultSwapStyle to SwapStyle.Default sets the default style to the htmx default, so it allows configuring the default swap style explicitly with a value even though the implication is that the value will be whatever the system default is
  • For reswaps the implication would be that the reswap wouldn't touch the underlying hx-swap swapping style, whatever it's default may be whether set within the markup or by system default
  • I'm not sure for AjaxContext if it would have any effect (?)

In some ways it's like setting a value to true/false/unset.

What it does also do is allows you to also build fluent chains off of SwapStyle.Default as well, so SwapStyle.Default.ShowNone() builds a reswap as "show:none" only. Likewise, SwapStyle.Default.After(TimeSpan.FromSeconds(1)).ScrollTop().ShowTransition().IgnoreTitle() builds modifiers only with no SwapStyle override.

Significant enhancements have been made to the SwapStyleBuilder class. The class has been sealed and several new methods have been added to provide more granular control over swap style commands. These include methods for controlling scroll direction, transition effects, title inclusion, and delay times for both swap and settle operations. Existing methods have also been updated with more detailed comments and improved parameter naming for better clarity.
@tanczosm tanczosm force-pushed the mat-response-additions branch from 1e5df1e to 76509fb Compare April 30, 2024 23:35
Enhanced the SwapStyleBuilder class by adding new methods to control viewport positioning after a swap operation. This includes methods for specifying whether the viewport should be positioned at the top or bottom of the swap target, and whether it should scroll to a focused element when a request completes. Also added corresponding tests to verify these new features.
New unit tests have been added to the `SwapStyleBuilderTests` class. These tests cover the `ShowOnTop`, `ShowOnBottom`, and `MixedShowOverrides` methods, ensuring they return correct values. The inheritance from `TestContext` has also been removed from this class.
@tanczosm
Copy link
Collaborator Author

tanczosm commented May 1, 2024

Here is the full set of builder methods:

  • SwapStyleBuilder AfterSwapDelay(TimeSpan time)
  • SwapStyleBuilder AfterSettleDelay(TimeSpan time)
  • SwapStyleBuilder Scroll(ScrollDirection direction, string? selector = null)
  • SwapStyleBuilder ScrollTop(string? selector = null)
  • SwapStyleBuilder ScrollBottom(string? selector = null)
  • SwapStyleBuilder IgnoreTitle(bool ignore = true)
  • SwapStyleBuilder IncludeTitle()
  • SwapStyleBuilder Transition(bool show)
  • SwapStyleBuilder IncludeTransition()
  • SwapStyleBuilder IgnoreTransition()
  • SwapStyleBuilder ScrollFocus(bool scroll = true)
  • SwapStyleBuilder PreserveFocus()
  • SwapStyleBuilder ShowOn(ScrollDirection direction, string? selector = null)
  • SwapStyleBuilder ShowOnTop(string? selector = null)
  • SwapStyleBuilder ShowOnBottom(string? selector = null)
  • SwapStyleBuilder ShowWindow(ScrollDirection direction)
  • SwapStyleBuilder ShowWindowTop()
  • SwapStyleBuilder ShowWindowBottom()
  • SwapStyleBuilder ShowNone()

@tanczosm
Copy link
Collaborator Author

tanczosm commented May 1, 2024

So remaining issues:

  • Finish Documentation (ready for review)
  • Can we add a Default SwapStyle which means effectively whatever htmx defaults to?
  • Any changes to method names?

Updated the SwapStyleBuilder class and its extension to improve code readability and maintainability. Changes include:
- Modified method descriptions to provide more accurate information about their functionality.
- Updated parameter names and descriptions for better understanding of their purpose.
- Enhanced remarks sections with additional details about the methods' behavior.
@egil
Copy link
Owner

egil commented May 1, 2024

So remaining issues:

  • Finish Documentation (ready for review)
  • Can we add a Default SwapStyle which means effectively whatever htmx defaults to?

Would it work if SwapStyle.Default implies what the user specifies in HtmxConfig.DefaultSwapStyle, or if that is unset, is the htmx default to. During rendering, the renderer and/or the builder can look at the config and replace an empty string, as the default is either specified in the config or is just what htmx would do anyway?

  • Any changes to method names?

I will go over them and report back.

Enhanced the SwapStyleBuilder's Scroll and ShowOn methods to allow for more precise control over scrollbar positioning after a content swap. The methods now accept an optional CSS selector parameter, which can be used to specify a target element for scrolling. If provided, the page will scroll to the top or bottom of the selected element after swapping content. Removed redundant code related to smooth animation of scrollbar position. Updated corresponding unit tests accordingly.
@tanczosm
Copy link
Collaborator Author

tanczosm commented May 1, 2024

I refactored the code a bit and just added a nullable selector for Scroll and ShowOn when selectors are used. I updated the list of methods.

@tanczosm
Copy link
Collaborator Author

tanczosm commented May 1, 2024

Would it work if SwapStyle.Default implies what the user specifies in HtmxConfig.DefaultSwapStyle, or if that is unset, is the htmx default to. During rendering, the renderer and/or the builder can look at the config and replace an empty string, as the default is either specified in the config or is just what htmx would do anyway?

The ordering of what swap style is used looks like this (in order of precedence):

  1. Swap style specified in hx-swap
  2. Swap style specified in DefaultSwapStyle configuration
  3. Swap style specified by htmx as the default (innerHTML)

For reswaps it is:

  1. Swap style specified in reswap (if exists)
  2. Swap style specified in hx-swap
  3. Swap style specified in DefaultSwapStyle configuration
  4. Swap style specified by htmx as the default (innerHTML)

The impact of adding SwapStyle.Default as an option would mean that it is possible that a developer could set HtmxConfig.DefaultSwapStyle to SwapStyle.Default. When emitting the configuration code, however, the value for DefaultSwapStyle cannot be 'default' as that is an invalid value. So one approach might be to override the setter on the configuration default swap style and ignore attempts to set it to SwapStyle.Default.

@egil
Copy link
Owner

egil commented May 1, 2024

The impact of adding SwapStyle.Default as an option would mean that it is possible that a developer could set HtmxConfig.DefaultSwapStyle to SwapStyle.Default. When emitting the configuration code, however, the value for DefaultSwapStyle cannot be 'default' as that is an invalid value. So one approach might be to override the setter on the configuration default swap style and ignore attempts to set it to SwapStyle.Default.

That makes sense. Lets do that.

Tried to pull the PR down and make some changes but it looks like I do not have permissions to push to your branch.

@egil
Copy link
Owner

egil commented May 1, 2024

Getting a ! [remote rejected] mat-response-additions -> refs/pull/23/head (deny updating a hidden ref) when I try to push to the PR. Here is a patch file instead.

Htmxor-c7a345e-fix tweaks to code docs, added default swap style.patch

@tanczosm
Copy link
Collaborator Author

tanczosm commented May 1, 2024

As far as the swap style builder is concerned, the build method returns the style and modifiers as a tuple. Would it be better for the builder to create strings so it could alternatively be used to create swaps for hx-get as well?

Your example was:

<div hx-swap=@SwapStyle.InnerHTML.ScrollTop() hx-get="..."></div>

I think for this to work I would have to end the chain with Build() or ToString().

<div hx-swap=@SwapStyle.InnerHTML.ScrollTop().Build() hx-get="..."></div>
<div hx-swap=@SwapStyle.InnerHTML.ScrollTop().ToString() hx-get="..."></div>

@egil
Copy link
Owner

egil commented May 1, 2024

btw. excellent work with the code docs. Really nice that users wont have to go to the htmx docs, they have all they need in the editor!

@egil
Copy link
Owner

egil commented May 1, 2024

The builder should implement a ToString() method that returns the output for the attribute. Then, if users are in debug mode and look at the builder, they will also see that value, which is a very nice bonus.

@tanczosm
Copy link
Collaborator Author

tanczosm commented May 1, 2024

The impact of adding SwapStyle.Default as an option would mean that it is possible that a developer could set HtmxConfig.DefaultSwapStyle to SwapStyle.Default. When emitting the configuration code, however, the value for DefaultSwapStyle cannot be 'default' as that is an invalid value. So one approach might be to override the setter on the configuration default swap style and ignore attempts to set it to SwapStyle.Default.

That makes sense. Lets do that.

Tried to pull the PR down and make some changes but it looks like I do not have permissions to push to your branch.

I added write access.

@egil
Copy link
Owner

egil commented May 1, 2024

@tanczosm
Copy link
Collaborator Author

tanczosm commented May 1, 2024

Think you need to do this to allow me to push: docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Looking into this. I don't have an "Allow edits and access to secrets by maintainers" option.

@egil
Copy link
Owner

egil commented May 1, 2024

ok, ill just merge this, create a PR on my own, and you can provide your suggestions :)

@egil egil merged commit d0e3e73 into egil:main May 1, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants